Work around rustc 1.93 perf regression with MaybeUninit#410
Conversation
emilio
left a comment
emilio
left a comment
There was a problem hiding this comment.
Some of the unsafe here seems gratuitous. Can you apply:
diff --git a/src/lib.rs b/src/lib.rs
index f7a2dff..446d736 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -653,7 +653,7 @@ impl<A: Array> SmallVecData<A> {
// memset over the whole struct. Initializing the field via a raw pointer write breaks
// the GVN constant-propagation chain and restores the expected codegen.
let mut data = SmallVecData {
- inline: unsafe { MaybeUninit::uninit().assume_init() },
+ inline: core::mem::ManuallyDrop::new(MaybeUninit::uninit()),
};
unsafe { core::ptr::addr_of_mut!(*data.inline).write(inline) };
data
@@ -723,7 +723,7 @@ impl<A: Array> SmallVecData<A> {
// `MaybeUninit::uninit()` as a global constant, which LLVM then collapses into a
// memset over the whole struct. Constructing with a fresh uninit payload first and
// then overwriting via a raw pointer write breaks the GVN chain.
- let mut data = SmallVecData::Inline(unsafe { MaybeUninit::uninit().assume_init() });
+ let mut data = SmallVecData::Inline(MaybeUninit::uninit());
match &mut data {
SmallVecData::Inline(a) => unsafe { core::ptr::write(a as *mut _, inline) },
_ => unsafe { core::hint::unreachable_unchecked() },Looks good to me with that.
|
I'm not sure why this is not happening with cargo bench, but the more broad test reduced from Firefox still does trigger the bug with your suggestion: https://rust.godbolt.org/z/Yq4rasTx4 |
| // memset over the whole struct. Initializing the field via a raw pointer write breaks | ||
| // the GVN constant-propagation chain and restores the expected codegen. | ||
| let mut data = SmallVecData { | ||
| inline: unsafe { MaybeUninit::uninit().assume_init() }, |
There was a problem hiding this comment.
Why is it valid to call assume_init() here? Docs (https://doc.rust-lang.org/stable/std/mem/union.MaybeUninit.html#safety) say that calling assume_init() on uninitialized data is immediate UB. And to me it looks like this data could be uninitialized?
There was a problem hiding this comment.
we're overwriting the data immediately.
There was a problem hiding this comment.
I might be wrong, but I thought you had to overwrite the data before calling assume_init() (and that this is the purpose of MaybeUninit: to allow you to have write access to an uninitialized buffer (so that you can initialize it) without that being UB)
There was a problem hiding this comment.
@nicoburns This also tripped me up when I looked at it, but it seems fine. It's not about overwriting the data, IMO. The types here are: MaybeUninit<ManuallyDrop<MaybeUninit<A>>>::uninit().assume_init().
It's always safe to assume_init a ManuallyDrop<MaybeUninit<..>>... It should be redundant, but see the discussion above, it seems to matter.
There was a problem hiding this comment.
Ah, there's an inner MaybeUninit? Yeah, I think that makes it fine. I would love to see a SAFETY comment to that effect if possible.
emilio
left a comment
emilio
left a comment
There was a problem hiding this comment.
Pretty gross, but I guess it's fine. Once this is fixed in a stable Rustc we should revert the change of course...
glandium
force-pushed
the
maybeuninit-workaround
branch
from
263ee34 to
99e1c6d
Compare
3 participants
Fixes #408